[Bugfix] - Changes in logic to delete share db by TejasRGitHub · Pull Request #1706 · data-dot-all/dataall

TejasRGitHub · 2024-11-19T21:25:42Z

Feature or Bugfix

Bugfix

Detail

Additional locking when revoking a dataset with the same database shared across various datasets
Modifications in checks to see if the principal / database combo is still shared else where

Relates

[Bug] One share revoke causes other associated shares to go into unhealthy state #1633

Security

Please answer the questions below briefly where applicable, or write N/A. Based on
OWASP 10.

Does this PR introduce or modify any input fields or queries - this includes
fetching data from storage outside the application (e.g. a database, an S3 bucket)? N/A
- Is the input sanitized?
- What precautions are you taking before deserializing the data you consume? N/A
- Is injection prevented by parametrizing queries?
- Have you ensured no eval or similar functions are used?
Does this PR introduce any functionality or component that requires authorization? N/A
- How have you ensured it respects the existing AuthN/AuthZ mechanisms?
- Are you logging failed auth attempts?
Are you using or adding any cryptographic features? N/A
- Do you use a standard proven implementations?
- Are the used keys controlled by the customer? Where are they stored?
Are you introducing any new policies/roles/users? N/A
- Have you used the least-privilege principle? How?

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

TejasRGitHub · 2024-11-19T22:19:35Z

Test cases:

Onboarded two dataset ( D1, D2 ) with the same common_db. Onboarded a consumption role ( C1 ). Environment has two roles ( Scientist, Engineers )

Scenerio 1:

Created a share S1 with table from C1 to D1 ✅ ( Checked if the tables are accessible via Athena )
Created a share S2 with table from C1 to D2 ✅ ( Checked if the tables are accessible via Athena )
Revoked share S2 and checked if the common_db_shared still exists in the account ✅
Queried tables from S1 to validate access and also checked if LF permissions for the principal exists ✅

Scenario 2:

Repeated steps 1 and 2.
Created another share S3 with D1 from environment role : Scientist ( i.e. creating share within the same environment )
Checked if access to the table exists for this role ✅
Now revoked S1 and check if the access in S3 and S2 are intact ✅ ( Checked that the permission to the principals exists and Athena query works)
Revoked S3 and checked if the access to S2 is present ✅ ( Checked that the permission to the principals exists and Athena query works)
Revoked S2 and checked now that the shared database is deleted ✅

TODO
Scenario 3:

Repeat steps 1 and 2,
Onboard another dataset (D3) in another enviironment where the database name is the same i.e. common_db
Now make a share S1 from C1 to this dataset ( D3 ) and also make a share S2 with D2 or D1
Revoke share items in share S2 and check if the common_shared db is still present ✅

backend/dataall/modules/shares_base/db/share_object_repositories.py

backend/dataall/modules/shares_base/services/sharing_service.py

dlpzx

The changes requested in https://github.com/data-dot-all/dataall/pull/1706/files#r1933588925 need to be fixed. I have not completed the full review because those changes might impact the whole implementation

backend/dataall/modules/s3_datasets/db/dataset_repositories.py

backend/dataall/modules/s3_datasets_shares/db/s3_share_object_repositories.py

...d/dataall/modules/s3_datasets_shares/services/share_processors/glue_table_share_processor.py

backend/dataall/modules/shares_base/services/sharing_service.py

TejasRGitHub · 2025-01-29T22:17:59Z

The changes requested in https://github.com/data-dot-all/dataall/pull/1706/files#r1933588925 need to be fixed. I have not completed the full review because those changes might impact the whole implementation

@dlpzx , I have made changes to fix the structure of the code.

There are lot of file which have changed when I did make lint with the newer version of ruff due to which you are seeing a lot of file which are updated. I have marked the files which are a part of this PR to make it easy to review PR

# Conflicts: # tests/modules/s3_datasets/test_dataset_glossary.py

backend/dataall/modules/shares_base/services/sharing_service.py

...d/dataall/modules/s3_datasets_shares/services/share_processors/glue_table_share_processor.py

dlpzx

Only some imports need to be removed. The rest looks good IMO.
I am testing the ruff changes in a separate PR. There are format changes that require testing

# Conflicts: # backend/dataall/modules/s3_datasets/aws/athena_table_client.py # backend/dataall/modules/s3_datasets_shares/aws/glue_client.py # tests/modules/s3_datasets_shares/tasks/test_s3_access_point_share_manager.py

TejasRGitHub · 2025-01-30T17:30:44Z

Only some imports need to be removed. The rest looks good IMO. I am testing the ruff changes in a separate PR. There are format changes that require testing

Thanks @dlpzx , removed unused imports and also synced PR branch with. the main. now the ruff formatting changes should not appear.

Also I tested out the code changes in the PR with the same tests cases as mentioned over here - #1706 (comment)

TejasRGitHub · 2025-01-31T16:32:43Z

@dlpzx , I ran make lint and also unit test and everythings passed on my end. Acc to me, I think now this PR is also ready to be merged

backend/dataall/modules/s3_datasets_shares/db/s3_share_object_repositories.py

dlpzx

Left one minor comment, the rest looks good~

TejasRGitHub · 2025-02-03T18:13:04Z

Left one minor comment, the rest looks good~

@dlpzx , Updated PR with correction

### Feature or Bugfix - Bugfix ### Detail In #1706 all the scenarios tested had multiple Datasets sharing a shared-Glue database. The issue is that when there is no common shared Glue database the lock will try to lock an empty list of resources, resulting in timeouts in `backend/dataall/modules/s3_datasets_shares/services/share_processors/glue_table_share_processor.py:248` ### Relates - #1706 ### Security Please answer the questions below briefly where applicable, or write `N/A`. Based on [OWASP 10](https://owasp.org/Top10/en/). - Does this PR introduce or modify any input fields or queries - this includes fetching data from storage outside the application (e.g. a database, an S3 bucket)? - Is the input sanitized? - What precautions are you taking before deserializing the data you consume? - Is injection prevented by parametrizing queries? - Have you ensured no `eval` or similar functions are used? - Does this PR introduce any functionality or component that requires authorization? - How have you ensured it respects the existing AuthN/AuthZ mechanisms? - Are you logging failed auth attempts? - Are you using or adding any cryptographic features? - Do you use a standard proven implementations? - Are the used keys controlled by the customer? Where are they stored? - Are you introducing any new policies/roles/users? - Have you used the least-privilege principle? How? By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

TejasRGitHub added 2 commits November 19, 2024 15:17

Fixes for data-dot-allgh-1633

71a399b

Removing unused comments

bb7d40f

Corrections

bf251f4

TejasRGitHub mentioned this pull request Dec 13, 2024

[Enhancement] Increase timeout for ECS share handling tasks / make it configurable #1742

Open

dlpzx requested review from dlpzx and petrkalos January 29, 2025 09:39

dlpzx reviewed Jan 29, 2025

View reviewed changes

backend/dataall/modules/shares_base/db/share_object_repositories.py Outdated Show resolved Hide resolved

dlpzx reviewed Jan 29, 2025

View reviewed changes

backend/dataall/modules/shares_base/db/share_object_repositories.py Outdated Show resolved Hide resolved

dlpzx reviewed Jan 29, 2025

View reviewed changes

backend/dataall/modules/shares_base/db/share_object_repositories.py Outdated Show resolved Hide resolved

dlpzx reviewed Jan 29, 2025

View reviewed changes

backend/dataall/modules/shares_base/services/sharing_service.py Show resolved Hide resolved

dlpzx requested changes Jan 29, 2025

View reviewed changes

Changes as per the review comments

0e467ce